fix: separate HTTP bind host from gRPC gateway target host#2824
fix: separate HTTP bind host from gRPC gateway target host#2824omer-topal merged 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds two new server configuration options: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/servers/server.go (1)
262-273:⚠️ Potential issue | 🟠 MajorThe new default gRPC dial host can break TLS handshakes.
Line 272 dials
server.http.grpc_target_host(default127.0.0.1), while Line 263 passessrv.NameOverride(which defaults to empty) tocredentials.NewClientTLSFromFile(). When the server name override is empty, gRPC uses the dial target hostname for TLS certificate verification. This means certificates issued for a DNS name will fail hostname verification against127.0.0.1, even though the gateway is connecting to the same local gRPC server. Ensure certificates match the dial target hostname or setserver.name_overrideto the certificate's CN/SAN when deploying with TLS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/server.go` around lines 262 - 273, When TLS is enabled, compute a single serverName variable and use it both when creating TLS credentials and when building the dial target so hostname verification uses the intended name: set serverName := srv.NameOverride; if serverName == "" then fallback to srv.HTTP.GRPCTargetHost; call credentials.NewClientTLSFromFile(srv.GRPC.TLSConfig.CertPath, serverName) and build targetAddr with net.JoinHostPort(serverName, srv.GRPC.Port) (instead of using srv.NameOverride in one place and srv.HTTP.GRPCTargetHost in the other) so the dial hostname matches the certificate CN/SAN.
🧹 Nitpick comments (1)
pkg/cmd/flags/serve.go (1)
32-37: Add a regression test for these new bindings.These lines are the only thing that makes the new knobs reachable from flags/env, and the config tests in this PR only cover YAML/default loading. A small table-driven test for
server-host/http-grpc-target-hostand the matchingPERMIFY_*vars would make this wiring much harder to break later.Also applies to: 90-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/flags/serve.go` around lines 32 - 37, Add a table-driven regression test (e.g., TestFlagEnvBindings) in the pkg/cmd/flags package that verifies viper.BindPFlag and viper.BindEnv wiring for the new knobs: "server-host" and "http-grpc-target-host" (and their env names PERMIFY_SERVER_HOST and PERMIFY_HTTP_GRPC_TARGET_HOST). For each case, set up the flags (simulate passing --server-host/--http-grpc-target-host), set the corresponding environment variable, call the same initialization that runs viper.BindPFlag/viper.BindEnv (the code in serve.go that calls viper.BindPFlag and viper.BindEnv), then assert viper.GetString("server.host") / viper.GetString("http.grpc_target_host") returns the expected value; reset viper and flags between cases to avoid cross-test pollution. Ensure the test covers both flag-precedence and env-only scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 288-292: The server default currently sets Host = "" which binds
to all interfaces; change the default to loopback-only by setting Host to
"127.0.0.1" (or "::1" if IPv6 desired) while leaving HTTP.GRPCTargetHost
(server.http.grpc_target_host) unchanged for internal gRPC dialing; update the
default in the config struct where Host and HTTP (Enabled, Port, GRPCTargetHost)
are initialized so the HTTP server binds to loopback by default but continues to
use GRPCTargetHost for the internal dial path.
---
Outside diff comments:
In `@internal/servers/server.go`:
- Around line 262-273: When TLS is enabled, compute a single serverName variable
and use it both when creating TLS credentials and when building the dial target
so hostname verification uses the intended name: set serverName :=
srv.NameOverride; if serverName == "" then fallback to srv.HTTP.GRPCTargetHost;
call credentials.NewClientTLSFromFile(srv.GRPC.TLSConfig.CertPath, serverName)
and build targetAddr with net.JoinHostPort(serverName, srv.GRPC.Port) (instead
of using srv.NameOverride in one place and srv.HTTP.GRPCTargetHost in the other)
so the dial hostname matches the certificate CN/SAN.
---
Nitpick comments:
In `@pkg/cmd/flags/serve.go`:
- Around line 32-37: Add a table-driven regression test (e.g.,
TestFlagEnvBindings) in the pkg/cmd/flags package that verifies viper.BindPFlag
and viper.BindEnv wiring for the new knobs: "server-host" and
"http-grpc-target-host" (and their env names PERMIFY_SERVER_HOST and
PERMIFY_HTTP_GRPC_TARGET_HOST). For each case, set up the flags (simulate
passing --server-host/--http-grpc-target-host), set the corresponding
environment variable, call the same initialization that runs
viper.BindPFlag/viper.BindEnv (the code in serve.go that calls viper.BindPFlag
and viper.BindEnv), then assert viper.GetString("server.host") /
viper.GetString("http.grpc_target_host") returns the expected value; reset viper
and flags between cases to avoid cross-test pollution. Ensure the test covers
both flag-precedence and env-only scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6b2739c-e58c-4fc9-9adc-538de14bfbe0
📒 Files selected for processing (8)
docs/setting-up/configuration.mdxexample.config.yamlinternal/config/config.gointernal/config/config_test.gointernal/servers/server.gopkg/cmd/config.gopkg/cmd/flags/serve.gopkg/cmd/serve.go
| Host: "", | ||
| HTTP: HTTP{ | ||
| Enabled: true, | ||
| Port: "3476", | ||
| Enabled: true, | ||
| Port: "3476", | ||
| GRPCTargetHost: "127.0.0.1", |
There was a problem hiding this comment.
Keep server.host loopback-only by default.
Setting Host to "" makes the HTTP server bind :3476, i.e. all interfaces. That broadens network exposure on upgrade even though this change only needs to decouple the gateway dial target from the bind address. Please keep the bind default on loopback and use server.http.grpc_target_host only for the internal gRPC dial path.
Suggested fix
Server: Server{
NameOverride: "",
- Host: "",
+ Host: "127.0.0.1",
HTTP: HTTP{
Enabled: true,
Port: "3476",
GRPCTargetHost: "127.0.0.1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Host: "", | |
| HTTP: HTTP{ | |
| Enabled: true, | |
| Port: "3476", | |
| Enabled: true, | |
| Port: "3476", | |
| GRPCTargetHost: "127.0.0.1", | |
| Host: "127.0.0.1", | |
| HTTP: HTTP{ | |
| Enabled: true, | |
| Port: "3476", | |
| GRPCTargetHost: "127.0.0.1", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/config.go` around lines 288 - 292, The server default
currently sets Host = "" which binds to all interfaces; change the default to
loopback-only by setting Host to "127.0.0.1" (or "::1" if IPv6 desired) while
leaving HTTP.GRPCTargetHost (server.http.grpc_target_host) unchanged for
internal gRPC dialing; update the default in the config struct where Host and
HTTP (Enabled, Port, GRPCTargetHost) are initialized so the HTTP server binds to
loopback by default but continues to use GRPCTargetHost for the internal dial
path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2824 +/- ##
==========================================
+ Coverage 82.63% 82.64% +0.02%
==========================================
Files 74 74
Lines 8126 8127 +1
==========================================
+ Hits 6714 6716 +2
+ Misses 893 892 -1
Partials 519 519 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
solves #2800 |
2 participants
Summary by CodeRabbit
New Features
server.hostconfiguration option to specify which interface/host the HTTP server binds tohttp-grpc-target-hostconfiguration option and environment variable to specify where the HTTP gateway connects to the local gRPC serverDocumentation